-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search in linked lists using a BTree #603
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, the approach for searching items is much cleaner than what I had started doing.
I think the low performance improvement we've noticed is coming from the fact that you don't have a way to know the length post insertion / deletion, hence the needless operations. We should see additional gains once this is addressed.
Also any reason why you didn't touch the AccessedAddresses
/ AccessedStorageKeys
access lists? They use the LinkedList
approach and also suffer from linear search.
let storage_mem = self | ||
.memory | ||
.get_preinit_memory(Segment::StorageLinkedList); | ||
self.storage.extend( | ||
StorageLinkedList::from_mem_and_segment(&storage_mem, Segment::StorageLinkedList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit heavy in terms of allocation, why don't we instead update the new accounts
/ storage
maps as we process the initial tries in preinitialize_linked_lists_and_txn_and_receipt_mpts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and also because of the iteration below which is linear and may be slow for large batches
They follow a slightly different logic that we already had struggle with when trying to merge access lists with linked lists in the kernel. But yes, this should be tackled in the future. I created issue #611 |
Co-authored-by: Robin Salen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alonso, LGTM! It now takes around ~500/700ns on my machine for searches, instead of up to 1.5ms for large batches on develop.
Replaces the linear search in the linked lists by searching on auxiliary Btrees of accounts and storage. Closes #391.